drm/vc4: txp: fix writeback dimension checks and normalize rotation#7292
drm/vc4: txp: fix writeback dimension checks and normalize rotation#7292name2965 wants to merge 2 commits intoraspberrypi:rpi-6.12.yfrom
Conversation
|
@6by9 ? |
6by9
left a comment
6by9
left a comment
There was a problem hiding this comment.
As per the comments on the original code below, there are two && which should be ||, but that is the full extent of the bug.
drm_rotation_simplify adds nothing to this check in my opinion because it doesn't understand transpose (downstream only thing). If it unpacked eg ROT_90 | REFLECT_X (or whichever combination it is) into ROT_0 | TRANSPOSE then it'd be lovely, but it doesn't and there is no need.
Expanding out the tests into multiple tests (some with no error message logged) makes the diff from mainline bigger, and hence a larger maintenance burden. Minimal changes are good here.
I agree that The cause of the problem occurring in This was definitely not a normal flow, so after debugging in detail to see why rotation was becoming NULL, I realized that because &drm_connector_funcs.reset was not defined separately to initialize rotation and the default function was still being used, rotation remained NULL until it was initialized separately in user space, and this continued up to atomic_check. Therefore, I will modify the existing patch and write and push an additional patch to resolve the rotation initialization issue. |
|
Oh, and if you need the reproduction code for the review, I will send it to you when I get home later. |
name2965
force-pushed
the
bugfix/txp-atomic-check
branch
2 times, most recently
from
20eefd0 to
fd890cf
Compare
name2965
force-pushed
the
bugfix/txp-atomic-check
branch
from
fd890cf to
7d54ee9
Compare
|
This is the reproduction code I wrote for testing. I modified the code reproduced in the video by removing as many hardcoded values as possible and reinforcing the logic to ensure it works well in other environments, but it may not function properly. |
pelwell
left a comment
pelwell
left a comment
There was a problem hiding this comment.
I'm happy with these commits (trusting that the reset method is necessary), but note that 6.12 is essentially frozen except for bug fixes - of which this is one.
Of course. I think that adding a custom reset function is the best way to prevent DMA OOB bug caused by this uninitialize rotation var. |
|
We've jumped on to the next LTS - 6.18. This is already shipping as our default |
6by9
left a comment
6by9
left a comment
There was a problem hiding this comment.
I'll give your test a quick run later today.
drivers/gpu/drm/vc4/vc4_txp.c
Outdated
| state->rotation = rotation; | ||
| } | ||
|
|
||
| __drm_atomic_helper_connector_reset(connector, state); |
There was a problem hiding this comment.
Based on the pattern with vc4_vec_connector_reset (in vc4_vec.c), it is safe to set the default after __drm_atomic_helper_connector_reset has been called.
The open coding of drm_atomic_helper_connector_reset can therefore be replaced by a call to it, and then check for a default value of rotation and set it.
There was a problem hiding this comment.
Based on the pattern with
vc4_vec_connector_reset(in vc4_vec.c), it is safe to set the default after__drm_atomic_helper_connector_resethas been called.The open coding of
drm_atomic_helper_connector_resetcan therefore be replaced by a call to it, and then check for a default value of rotation and set it.
In VC4, since the drm_connector_state struct is not subclassed separately, I was debating whether to call drm_atomic_helper_connector_reset() first.
However, I was concerned that initializing the connector->state pointer beforehand might cause concurrency issues, so I safely implemented it to call reset after the state is initialized.
Will there really be no problems if I call drm_atomic_helper_connector_reset() first?
There was a problem hiding this comment.
If there were an issue, the mainline devs wouldn't have written:
https://github.com/torvalds/linux/blob/master/drivers/gpu/drm/vc4/vc4_vec.c#L381-L385
static void vc4_vec_connector_reset(struct drm_connector *connector)
{
drm_atomic_helper_connector_reset(connector);
drm_atomic_helper_connector_tv_reset(connector);
}
and the same in https://github.com/torvalds/linux/blob/master/drivers/gpu/drm/sun4i/sun4i_tv.c#L404-L408
static void sun4i_tv_connector_reset(struct drm_connector *connector)
{
drm_atomic_helper_connector_reset(connector);
drm_atomic_helper_connector_tv_reset(connector);
}
The implementation of drm_atomic_helper_connector_tv_reset is doing exactly the same thing as you, going in to set properties to their default values https://github.com/torvalds/linux/blob/master/drivers/gpu/drm/drm_atomic_state_helper.c#L504-L577
There was a problem hiding this comment.
That make sense thanks! I will write the patch right away, test it, and push new commit.
name2965
force-pushed
the
bugfix/txp-atomic-check
branch
2 times, most recently
from
0c4842d to
950266e
Compare
6by9
left a comment
6by9
left a comment
There was a problem hiding this comment.
Other than being against 6.12, that looks good. Cherry-picked to 6.18 and it passes your test case.
Ignore checkpatch's complaint on u64 instead of uint64_t in this case.
…tomic_check Since incorrect conditional operator was used in vc4_txp_atomic_check(), the check may be bypassed if only one of the width or height does not match. To prevent this, the conditional operator must be corrected. Fixes: c5d3a57 ("drm/vc4: txp: Add a rotation property to the writeback connector") Signed-off-by: Jeongjun Park <[email protected]>
…m function In the previous commit, we added a rotation parameter to be used in the connector, but because we are still using the default reset function without implementing a custom reset function to properly initialize it, the rotation variable remains NULL until it is initialized directly in userspace. To prevent this, we must implement a custom reset function that properly initializes the rotation parameter. Fixes: 30c7044 ("drm: Add a rotation parameter to connectors.") Signed-off-by: Jeongjun Park <[email protected]>
name2965
force-pushed
the
bugfix/txp-atomic-check
branch
from
950266e to
81efcb8
Compare
4 participants
If the rotation value is 0, it can be compared to an accurate bitmask, thereby bypassing size validation; furthermore, frame buffer size validation is so inadequate that it cannot reliably filter out write buffers that are too small.
This results in a DMA OOB vulnerability on Raspberry Pi models prior to the 5 that lack an IOMMU, and I have successfully reproduced this vulnerability.
I tested this immediately by installing the Raspberry Pi OS 64-bit on a Raspberry Pi 4 Model B rev 1.2 using Imager.
This issue occurs on Raspberry Pi without IOMMU, and DMA OOB can be easily reproduced in any kernel version where vulnerable code exists, in addition to the kernel version I reproduced.
repro.mp4